Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip generating unnecessary @supports #878

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sapphi-red
Copy link
Contributor

This PR introduces TargetsWithSupportsScope struct that tracks the parent @supportss. This struct has current field that contains the current target that is based on the target option and the parent @supportss.

This makes lightningcss to skip generating unnecessary @supports.

closes #711

SupportsCondition::Declaration { property_id, value } => match property_id {
PropertyId::Color => Some(match value.as_ref() {
COLOR_P3_SUPPORTS_CONDITION => Features::P3Colors | Features::ColorFunction,
COLOR_LAB_SUPPORTS_CONDITION => Features::LabColors,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would only work if the value is an exact match. Perhaps a future enhancement would be to attempt to parse the value, and extract the features from that. For example, if any lab() color was used, set that feature rather than only lab(0% 0 0)

Copy link
Contributor Author

@sapphi-red sapphi-red Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it would be nice to expand this detection in future.

src/targets.rs Outdated
#[derive(Debug)]
pub(crate) struct TargetsWithSupportsScope {
input_targets: Targets,
supports_scope_features: IndexSet<Features>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably get away with just a Vec here. Since Features is a bit set, it should only accumulate new items the deeper you go. So you could track each @supports you enter in a stack, and modify current in place:

fn enter_supports(&mut self, features: Features) -> bool {
  if features.is_empty() || self.current.exclude.contains(features) {
    // Already excluding all features
    return false;
  }

  self.stack.push(supported);
  self.current.exclude.insert(features);
  true
}

fn exit_supports(&mut self) {
  if let Some(last) = self.stack.pop() {
    self.current.exclude.remove(last);
  }
}

Then you wouldn't need to iterate over the stack each time and could avoid the hashing cost of IndexSet.

Do you think this would work or am I missing something?

Copy link
Contributor Author

@sapphi-red sapphi-red Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true! I updated the code to use Vec.
I added this line to make cases like what I added as a test case work.
394930a#diff-05702e78430359853e5b38404035fa394540368bee185a2cdc936a62b9cd65b0R271

@@ -225,6 +248,43 @@ impl Targets {
}
}

#[derive(Debug)]
pub(crate) struct TargetsWithSupportsScope {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be part of Targets?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean putting the fields in Targets and making it stateful?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transpilation generates unnecessary fallback inside @supports
2 participants